-
Notifications
You must be signed in to change notification settings - Fork 585
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add a workaround for React Native Proxy
objects pre-JSI not working properly with JSC.
#4541
Conversation
b891394
to
3f29ae0
Compare
284e762
to
621a89d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My only comment is about naming of a constant and it should not block merging this PR.
lib/mutable-subscription-set.js
Outdated
// work fine, by reverting PR #4541. | ||
const instanceMethods = (realmConstructor) => ({ | ||
add(query, options) { | ||
return this._add(query[symbols.REALM_REACT_PROXIED_OBJECT] || query, options); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have some reservations about REALM_REACT_PROXIED_OBJECT
- it is leaking one package to another and in the opposite direction of the dependency. Could we find a neutral name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps ORIGINAL_REALM_COLLECTION
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went with PROXY_TARGET
as per Kræn's suggestion
Closing in favour of waiting for const people = useQuery(realm.objects('Person'));
// ...
realm.subscriptions.update(subs => {
subs.add(people); // this will error
}); becomes const results = realm.objects('Person');
const people = useQuery(people);
// ...
realm.subscriptions.update(subs => {
subs.add(results); // no error as we are passing in the Realm.Results instance directly
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like a very good way to getting around this issue. I would, however, like to see a test for this so that we can ensure it still works after v11 when we remove this code.
lib/mutable-subscription-set.js
Outdated
// work fine, by reverting PR #4541. | ||
const instanceMethods = (realmConstructor) => ({ | ||
add(query, options) { | ||
return this._add(query[symbols.REALM_REACT_PROXIED_OBJECT] || query, options); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps ORIGINAL_REALM_COLLECTION
lib/mutable-subscription-set.js
Outdated
|
||
remove(query) { | ||
return this._remove(query[symbols.REALM_REACT_PROXIED_OBJECT] || query); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems we are missing some linter fixes in this file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have just minor suggestions.
621a89d
to
950da15
Compare
950da15
to
fb7983e
Compare
@takameyer I like the idea of adding a test but I am struggling to think of a good way to do it, because in order to get access to
If you have other ideas let me know though as I agree it would be good, but I'm not sure if it's worth a fairly major bit of work to achieve. I might be missing something easier. As is, we'll have to remember to test it again with v11 which I agree is not ideal! |
What, How & Why?
As described in #4507 (comment), React Native
Proxy
objects which do not use JSI do not seem to be compatible with JSC – it doesn't "unwrap" them properly so can't see the wrapped type.This means that you cannot pass a result from
useQuery
in@realm/react
to the subscription set mutation functions, as it is wrapped in a Proxy.This workaround stores the unproxied Results as a non-enumerable field on the proxied object, so that we can check for its presence and pass that to C++ instead of the proxied version when calling subscription set mutation functions.
Note that this problem seems to go away with JSI in
v11
(regardless of whether Hermes is being used) so another option is that we just accept the issue for now, and wait untilv11
becomesmaster
This closes #4507
☑️ ToDos
Compatibility
label is updated or copied from previous entryBreaking
label has been applied or is not necessaryIf this PR adds or changes public API's: